Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Adds datetime parsing to Request instance #39945

Merged
merged 12 commits into from
Dec 17, 2021
Merged

[8.x] Adds datetime parsing to Request instance #39945

merged 12 commits into from
Dec 17, 2021

Conversation

DarkGhostHunter
Copy link
Contributor

@DarkGhostHunter DarkGhostHunter commented Dec 8, 2021

What?

Allows to conveniently retrieve an input from the Request as a Carbon object (or DateTimeInterface). It uses the Date facade behind the scenes.

public function post(Requets $request)
{
    // ...

    $post->publish_at = $request->date('publish_at')->addHour()->startOfHour();

    $post->save();

    return back();
}

Why?

Because you can quickly get a lot of Carbon or Date calls to parse the date from the request, specially from different keys.

// Before
if ($date = $request->input('when')) {
    $date = Carbon::parse($datetime);
}

// After
$date = $request->date('when');

It supports for custom formats (otherwise it will try to parse it).

$timestamp = $request->date('when', 'U');

And also supports timezoning.

$timestamp = $request->date('when', 'U', 'America/Santiago');

Since the input can be null, it's easier to create defaults:

$now = $request->date('when', 'U') ?? now();

BC?

No, only additive.

@devfrey
Copy link
Contributor

devfrey commented Dec 8, 2021

This example is not very accurate:

if ($datetime = $request->input('when')) {
    $datetime = Carbon::parse($datetime);
}

It can already be written like this:

$datetime = Carbon::make($request->input('when'));

... which does not seem too bad to me.

@DarkGhostHunter
Copy link
Contributor Author

This example is not very accurate:

if ($datetime = $request->input('when')) {
    $datetime = Carbon::parse($datetime);
}

It can already be written like this:

$datetime = Carbon::make($request->input('when'));

... which does not seem too bad to me.

Same case for collect(). Also, you're accounting the input is not null.

@devfrey
Copy link
Contributor

devfrey commented Dec 9, 2021

Same case for collect().

I'm not sure what you mean by this?

Also, you're accounting the input is not null.

That's not true. Carbon::make(null) returns null. It's different from Carbon::parse().

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Dec 9, 2021

Just reworked it to return null if the datetime is invalid.

Also, Carbon::make(null) doesn't accept a timezone.

@crnkovic
Copy link
Contributor

I like the idea as I needed this many times, but you should probably take care of few stuff.

First, not sure why the use of Date facade, when DateTime and/or Carbon classes are sufficient for this, IMHO. Also, Carbon throws an InvalidFormatException, which I think is more expressive than just catching all invalid argument exceptions. Likewise, createFromFormat will return false on failure, so this should also be taken care of as the typehint of your date() method is Carbon or null.

@DarkGhostHunter
Copy link
Contributor Author

First, not sure why the use of Date facade when DateTime and/or Carbon classes are sufficient for this, IMHO

The developer may be using another library. Otherwise, the Date facade wouldn't exists. I'm just using what is available. Even $model->freshTimestamp() uses the Date facade behind the scenes.

Also, Carbon throws an InvalidFormatException, which I think is more expressive than just catching all invalid argument exceptions.

Same case as above. InvalidFormatException is exclusive to the Carbon library, as far as I know. Can't catch what doesn't exists, if it doesn't.

Likewise, createFromFormat will return false on failure, so this should also be taken care of as the typehint of your date() method is Carbon or null.

I would back to my original idea: if the format or the value to be formatted is invalid, throw an exception. It will return null only if the input is null or doesn't exists.

@crnkovic
Copy link
Contributor

I would back to my original idea: if the format or the value to be formatted is invalid, throw an exception. It will return null only if the input is null or doesn't exists.

Yeah okay makes sense, if it can't be parsed by Carbon then should throw an exception just like Carbon would.

@taylorotwell
Copy link
Member

I would back to my original idea: if the format or the value to be formatted is invalid, throw an exception. It will return null only if the input is null or doesn't exists.

@DarkGhostHunter I agree that this should be the behavior.

@taylorotwell taylorotwell marked this pull request as draft December 16, 2021 15:29
@DarkGhostHunter DarkGhostHunter marked this pull request as ready for review December 16, 2021 18:46
@DarkGhostHunter
Copy link
Contributor Author

It's ready for review.

@taylorotwell taylorotwell merged commit 51e358c into laravel:8.x Dec 17, 2021
@DarkGhostHunter DarkGhostHunter deleted the feat/request-datetime branch December 17, 2021 15:38
@DarkGhostHunter
Copy link
Contributor Author

👍

* @param string|null $tz
* @return \Illuminate\Support\Carbon|null
*/
public function date($key, $format = null, $tz = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not $timezone?

@iKlsR
Copy link

iKlsR commented Mar 5, 2022

I like this but I wish the function handled defaults better sans coalescing when the input isn't null so one could benefit more from the parsing upfront and I think the timezone is not needed in most cases so it could be optional in an array and use the param space for the default.

// Old
$startDate = Carbon::parse(Request::input('from', Carbon::now()->subYears(5)))->format('Ymd');
$endDate = Carbon::parse(Request::input('to', $lastTradeDate))->format('Ymd');

// New
$startDate = Request::date('from', 'Ymd') ?? Carbon::now()->subYears(5)->format('Ymd');
$endDate = Request::date('to', 'Ymd') ?? Carbon::parse($lastTradeDate)->format('Ymd'); 

// Expected?
$startDate = Request::date('from', ['Ymd'], Carbon::now()->subYears(5));
$endDate = Request::date('to', ['Ymd', 'America/Santiago'], $lastTradeDate);
$otherDate = Request::date('other', ['Ymd', 'America/Santiago']);
$anotherDate = Request::date('another', 'Ymd');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants